Skip to content

chore: Rework CodeViewer to Extract Trigger Button & Remove showInlineContent #1527

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
camielvs merged 1 commit into from
Jan 16, 2026
Merged

chore: Rework CodeViewer to Extract Trigger Button & Remove showInlineContent #1527

camielvs merged 1 commit into from
Jan 16, 2026

Conversation

@camielvs
Copy link
Collaborator

@camielvs camielvs commented Dec 10, 2025 •
edited
Loading

Description

Rework the CodeViewer so the inline trigger button for fullscreen-only is not embedded into the CodeViewer component itself. More specifically, this PR targets the removal of the showInlineContent prop.

The showInlineContent prop turns CodeViewer into a button that opens the actual code viewer component in fullscreen. This paradigm does not make much sense and is restrictive. e.g. we currently cannot open the codeviewer in fullscreen via a callback.

This PR removes showInlineContent and the related button, instead extracting that button into a new ViewYamlButton which can be reliably used anywhere in the app. The component now focusses on rendering the code viewer UI. The introduction of a fullscreen prop allows us to force it to render in fullscreen-mode only, like the original showInlineContent was doing. The existing behaviour in non-fullscreen mode is unchanged (it can still toggle fullscreen at will).

Note: I didn't want to get bogged down with trying to move all the divs in CodeViewer onto BlockStack. I feel that given it's a bespoke component it's probably okay for now to leave as-is. I did, however, clean up a couple of icons and css.

Related Issue and Pull requests

Type of Change

  • Cleanup/Refactor

Checklist

  • I have tested this does not break current pipelines / runs functionality
  • I have tested the changes on staging

Screenshots (if applicable)

No change to app functionality. Code architecture update only. Confirm that the CodeViewer still behaves exactly as expected.

Test Instructions

Additional Comments

This was referenced Dec 10, 2025
Merged
Merged
Merged
Merged
Merged
Merged
@camielvs Graphite App
Copy link
Collaborator Author

camielvs commented Dec 10, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@camielvs camielvs mentioned this pull request Dec 10, 2025
Closed
3 tasks
@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch from 541ae4c to dd7fc57 Compare December 10, 2025 22:50
@camielvs camielvs force-pushed the 12-10-rework_action_buttons_into_new_action_framework branch from 8737a95 to 73c5435 Compare December 10, 2025 22:50
@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch 3 times, most recently from 13b3154 to a243c75 Compare December 10, 2025 23:31
@camielvs camielvs changed the base branch from 12-10-rework_action_buttons_into_new_action_framework to graphite-base/1527 December 10, 2025 23:40
@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch from a243c75 to 1139464 Compare December 10, 2025 23:41
@camielvs camielvs changed the base branch from graphite-base/1527 to 12-09-cleanup_rundetails December 10, 2025 23:41
@camielvs camielvs force-pushed the 12-09-cleanup_rundetails branch from 73e7131 to 2a0ccca Compare December 11, 2025 00:23
@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch from 1139464 to 1dd8730 Compare December 11, 2025 00:23
@camielvs camielvs force-pushed the 12-09-cleanup_rundetails branch from 2a0ccca to f8e3134 Compare December 11, 2025 00:34
@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch from 1dd8730 to 0e92bae Compare December 11, 2025 00:34
@camielvs camielvs force-pushed the 12-09-cleanup_rundetails branch from f8e3134 to e677909 Compare December 11, 2025 00:36
@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch 2 times, most recently from 19707ab to 726344b Compare December 11, 2025 00:51
@camielvs camielvs force-pushed the 12-09-cleanup_rundetails branch 2 times, most recently from 65234bf to 7617cac Compare December 11, 2025 00:56
@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch from 726344b to 5b89d37 Compare December 11, 2025 00:56
@camielvs camielvs force-pushed the 12-09-cleanup_rundetails branch from 7617cac to c74095b Compare December 11, 2025 01:02
@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch 2 times, most recently from 5f2112e to 0d02f62 Compare December 11, 2025 20:26
@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch from a5ce09e to 47f036b Compare December 18, 2025 20:59
@camielvs camielvs force-pushed the 12-09-cleanup_rundetails branch from ed9558d to 1bccc0a Compare December 18, 2025 20:59
@camielvs camielvs changed the base branch from 12-09-cleanup_rundetails to graphite-base/1527 December 18, 2025 21:01
@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch from 47f036b to 661532b Compare December 18, 2025 21:02
@graphite-app graphite-app bot changed the base branch from graphite-base/1527 to master December 18, 2025 21:02
@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch from 661532b to eba55a1 Compare December 18, 2025 21:03
@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch 3 times, most recently from 3cf9d11 to 1d430eb Compare January 13, 2026 00:22
Mbeaulne
Mbeaulne previously requested changes Jan 14, 2026
View reviewed changes
Copy link
Collaborator

@Mbeaulne Mbeaulne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! I'm not sure I follow the benefit of this refactor. What's the use case for controlling fullscreen externally?

To me, fullscreen feels like an internal concern of the code viewer - user clicks the button, it goes fullscreen. I don't see when we'd need the parent to manage that state.

Breaking it up means more wiring every time we use it, and it spreads related logic across components. Unless there's a specific need I'm missing, I'd prefer to keep the code viewer self-contained.

What's driving this change?

@camielvs Graphite App
Copy link
Collaborator Author

camielvs commented Jan 14, 2026

@Mbeaulne I discussed the remainder of this stack with @maxy-shpfy this morning and we are going to try a different approach.

I am going to put the rest of the stack into draft while exploring & then probably close these PRs.

@camielvs camielvs marked this pull request as draft January 14, 2026 20:38
@camielvs camielvs mentioned this pull request Jan 15, 2026
Merged
3 tasks
@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch from 1d430eb to 09b0dc1 Compare January 15, 2026 04:03
@camielvs camielvs changed the title Rework CodeViewer Implementation chore: Rework CodeViewer Implementation to Extract Button Jan 15, 2026
@camielvs camielvs changed the title chore: Rework CodeViewer Implementation to Extract Button chore: Rework CodeViewer Implementation to Extract Trigger Button Jan 15, 2026
@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch 2 times, most recently from 448e775 to 950e030 Compare January 15, 2026 04:25
@camielvs camielvs marked this pull request as ready for review January 15, 2026 04:26
@camielvs camielvs requested a review from Mbeaulne January 15, 2026 04:26
@camielvs Graphite App
Copy link
Collaborator Author

camielvs commented Jan 15, 2026 •
edited
Loading

@Mbeaulne I have reworked this PR. Fullscreen functionality is no longer extracted from the Code Viewer and remains a concern of the code viewer itself. This simplifies things greatly.

The objective is to remove showInlineContent because it doesn't make much sense to have the trigger button embedded within the CodeViewer component itself. The motivation comes from a desire to simplify and standardize how our buttons in the Context Panel are defined & rendered.

What I've done now is rework the component to accept a fullscreen prop which renders the code viewer in fullscreen and then extracted the trigger button into ViewYamlButton. This should give us more flexibility, better separation of concerns and more stability for the CodeViewer in the future.

@camielvs camielvs changed the title chore: Rework CodeViewer Implementation to Extract Trigger Button chore: Rework CodeViewer to Extract Trigger Button & Remove showInlineContent Jan 15, 2026
maxy-shpfy
maxy-shpfy approved these changes Jan 15, 2026
View reviewed changes
Copy link
Collaborator

@maxy-shpfy maxy-shpfy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch from 950e030 to 260200e Compare January 15, 2026 19:42
@camielvs Graphite App
Copy link
Collaborator Author

camielvs commented Jan 16, 2026 •
edited
Loading

Merge activity

  • Jan 16, 12:35 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jan 16, 12:36 AM UTC: Graphite rebased this pull request as part of a merge.
  • Jan 16, 12:37 AM UTC: @camielvs merged this pull request with Graphite.

@camielvs camielvs force-pushed the 12-10-rework_codeviewer_implementation branch from 260200e to 84c3fc7 Compare January 16, 2026 00:35
@camielvs camielvs merged commit 80ffdbe into master Jan 16, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maxy-shpfy maxy-shpfy maxy-shpfy approved these changes

@Mbeaulne Mbeaulne Awaiting requested review from Mbeaulne

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@camielvs @Mbeaulne @maxy-shpfy